feat(gatsby): Add support for relative links #24054

ascorbic · 2020-05-13T13:24:34Z

Currently gatsby-link doesn't support relative paths, unlike the underlying @reach/router. This is because we need to do a lot more than navigation, particularly loading of resources. This is confusing to users, and people have long requested that we add support (e.g. #6945).

This PR adds support for relative links. It does this by resolving the links, both at runtime and build time, relative to the current route. It handles pathPrefix, but I've not yet tested it with client-only routes, which is why it's a draft. It follows the @reach/router behaviour of treating relative links as if the current page has a trailing slash. i.e linking from /foo/bar to .. resolves to /foo not / as it would with a POSIX path.

packages/gatsby-link/src/index.js

blainekasten · 2020-05-13T13:48:10Z

packages/gatsby/cache-dir/find-path.js

+  }
+  // Calculate path relative to current location, adding a trailing slash to
+  // match behavior of @reach/router
+  return new URL(path, window.location.href.replace(/([^/])$/, `$1/`)).pathname


URL constructor is not supported in IE11. https://caniuse.com/#search=url, does polyfiling take care of that for us?

Yeah, I wondered about that, but our docs says that we have babel auto-polyfills so I guessed it was ok

blainekasten

This is a really awesome start! I think the direction is looking great!

pvdz

The hunt for regexes is on.

packages/gatsby-link/src/index.js

pvdz · 2020-05-13T13:35:55Z

packages/gatsby-link/src/index.js

-  return [__PATH_PREFIX__].concat([path.replace(/^\//, ``)]).join(`/`)
+  return isRelativePath(path)
+    ? path
+    : [__PATH_PREFIX__].concat([path.replace(/^\//, ``)]).join(`/`)


Not your code, but if you're willing to drop the regex:

There's no need for the regex here. Not sure if the absolute path can safely assumed to start with / after the isRelative check, otherwise you can simplify that logic to only .slice(1) there. (That func only tests the opposite.)

Suggested change

: [__PATH_PREFIX__].concat([path.replace(/^\//, ``)]).join(`/`)

: `${__PATH_PREFIX__}/${path.startsWith('/')?path.slice(1):path`

sort of but there's browser support to consider for startsWith. It's a better API (that may not have existed or been implemented broadly at the time of this code change), but I'm not convinced it's worth making the change when it's functionally the same thing and more supported.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/startsWith

I'd assume we're polyfilling it already

🤷 are we comfortable going forward with an assumption? What's the benefit of changing this code path?

The reason I said I assumed it would be polyfilled is because the docs say we automatically include them. The benefit is performance. Regexes are really expensive. We could in theory replace it with a substring comparison instead, but that's a lot less readable, and by the time we've used it a few times will be larger than the polyfill (and unlike the polyfill it won't be removed if people target new browsers).

My other suggestions use startsWith and endsWith as well, fwiw.

If this is really a concern then yeah, don't do it. I too thought the polyfills are in place for this.

packages/gatsby-link/src/index.js

packages/gatsby/cache-dir/find-path.js

packages/gatsby-link/src/index.js

packages/gatsby/cache-dir/find-path.js

pvdz · 2020-05-13T17:52:05Z

💜 the changes, thank you :)

pieh · 2020-05-13T21:04:37Z

Let's make sure to add some test cases for relative links in
https://github.com/gatsbyjs/gatsby/blob/master/e2e-tests/development-runtime/cypress/integration/navigation/linking.js and https://github.com/gatsbyjs/gatsby/blob/master/e2e-tests/production-runtime/cypress/integration/1-production.js as this PR will get closer to "done"

Also please make sure that prefetching (___loader.enqueue calls) also have adjusted args - it might be a bit messy with those right now as those are not in render function so there might be need to move things around - maybe we could encapsulate relative path handling above GatsbyLink itself?

so current

export default React.forwardRef((props, ref) => (
  <GatsbyLink innerRef={ref} {...props} />
))

would be something like

export default React.forwardRef(({to, ...restOfProps}, ref) => (
<Location>
  {({ location }) => {
    const adjustedTo = magic(to, location)
    return <GatsbyLink innerRef={ref} to={adjustedTo} {...restOfProps} />
  })
</Location>
))

so GatsbyLink itself already get adjusted to prop and we don't have to make any changes there?

… feat/relative-links

packages/gatsby-link/src/index.js

ascorbic · 2020-05-15T09:47:31Z

I have some questions about link behaviour. This table shows how it behaves right now:

From page	Link	Current	Proposed
/	/foo	/foo	/foo
/	foo	/foo*	/foo
/	./	/foo*	/foo
/foo/	./bar	/bar/*	/foo/bar/
/foo	./bar	/bar*	/foo/bar
/foo/	bar	/bar/*	/foo/bar/ ?
/foo	bar	/bar*	/foo/bar ?
/foo/bar/	baz	/baz/*	/foo/bar/baz/ ?
/foo/bar	baz	/baz*	/foo/bar/baz ?
/foo/bar/	../baz	/baz/*	/foo/baz/
/foo/bar	../baz	/baz*	/foo/baz

* Gives warning about external links
? Not sure whether it should warn

All changed behaviour was already giving a warning. There are a few slightly confusing cases, which are due to how @reach/router handles trailing slashes. Because of how ambigious they can be, Reach treats them as if they all have a trailing slash, so linking from /foo/bar to ./baz will take you to foo/bar/baz, not /foo/baz as it would in POSIX. They give their rationale here, which I find persuasive but does potentially cause confusion so would need documentation.

My main question is whether we should warn for any of these now, particularly ones that are potentially ambiguous like a bare link with no inital slash or dot like foo

sidharthachatterjee · 2020-05-19T08:41:59Z

@ascorbic Tests are great! Thank you for those. Do we need to test this with gatsby-plugin-offline?

sidharthachatterjee · 2020-05-19T08:46:50Z

Canary is out at

Successfully published:
 - [email protected]+545ff9d68
 - [email protected]+545ff9d68
lerna success published 6 packages

sidharthachatterjee

Looks great!

wardpeet · 2020-05-19T12:19:13Z

We do not have a sizebot yet 😛
Just saying, this PR adds 1.2kb do the bundle. Mostly startWith polyfill. No big deal

sidharthachatterjee · 2020-05-19T12:30:04Z

@wardpeet You are our sizebot 😛

AishaBlake · 2020-05-19T18:33:19Z

@ascorbic Pulling my Slack comment out into the open! Would you mind updating the linking between pages doc as part of this PR? Let me know if you'd like a little direction there! I just want to make sure we're correctly describing the component's behavior wherever it's mentioned.

docs/docs/linking-between-pages.md

Ekwuno

This looks good to me. Thanks @ascorbic for documenting.

sidharthachatterjee

💥

robinmetral · 2020-06-20T07:08:15Z

packages/gatsby-link/src/index.js

+      <Location>
+        {({ location }) => {
+          const prefixedTo = rewriteLinkPath(to, location.pathname)
+          return isLocalLink(prefixedTo) ? (


@ascorbic did this also add support for external links?

I'm asking because the docs have long warned that should only be used for local links.

I think it's a nice feature, until now I've had to implement a similar workaround in a wrapper around to render either a Gatsby Link for local links or an anchor for others.

However this PR misses two things if this really is adding support for external links:

we need to update the docs for it

we need to remove this warning:

gatsby/packages/gatsby-link/src/index.js

Lines 169 to 173 in be86318

if (process.env.NODE_ENV !== `production` && !isLocalLink(to)) {

console.warn(

`External link ${to} was detected in a Link component. Use the Link component only for internal links. See: https://gatsby.dev/internal-links`

)

}

@robinmetral This change handles them more gracefully, but we still discourage their use, which is why we still have the warning. It means we can change the behaviour in future without making it a breaking change.

Thanks @ascorbic, got it. So as far as I understood, I can basically use Link for external links now, but I'll continue getting warnings.

What would be missing to make this a more officially supported feature? I can help if necessary, I'd love to get rid of my custom wrapper 😛

You can continue to use them, but risk having us change behaviour in the future. For example, adding relative link support broke some existing links without leading slashes, which had worked but with warnings.

fabianhaglund · 2020-07-09T12:46:54Z

Thank you heaps for this!

Add support for relative links

0d5f7a4

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 13, 2020

ascorbic added topic: reach/router* and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 13, 2020

blainekasten reviewed May 13, 2020

View reviewed changes

packages/gatsby-link/src/index.js Show resolved Hide resolved

blainekasten reviewed May 13, 2020

View reviewed changes

Review fixes

741f637

pvdz reviewed May 13, 2020

View reviewed changes

Changes from review + handle invalid links better

43af755

ascorbic requested a review from pvdz May 13, 2020 17:14

Changes from review + handle invalid links better

a87848c

pvdz reviewed May 13, 2020

View reviewed changes

packages/gatsby-link/src/index.js Outdated Show resolved Hide resolved

pvdz reviewed May 13, 2020

View reviewed changes

packages/gatsby-link/src/index.js Outdated Show resolved Hide resolved

pvdz reviewed May 13, 2020

View reviewed changes

packages/gatsby/cache-dir/find-path.js Outdated Show resolved Hide resolved

ascorbic added 2 commits May 14, 2020 09:34

Merge branch 'feat/relative-links' of github.com:gatsbyjs/gatsby into…

0e6a1aa

… feat/relative-links

Ensure path is tarnslated in more places

9e9530d

pieh reviewed May 14, 2020

View reviewed changes

packages/gatsby-link/src/index.js Outdated Show resolved Hide resolved

ascorbic added 5 commits May 15, 2020 12:09

Simplify link checks by explicitly checking for http(s)

a2fab60

Fix missing rewrite

750c9a8

Add tests and clear up edge cases

ed65316

Use explicit protocol list for detecting external links (sorry gopher)

972eed5

Merge branch 'master' into feat/relative-links

efd0fca

ascorbic marked this pull request as ready for review May 18, 2020 13:07

ascorbic requested a review from a team as a code owner May 18, 2020 13:07

ascorbic added 2 commits May 18, 2020 16:21

Don't double-prefix absolutified paths

80bec26

Add e2e tests

c7137b0

Fix calls to deprecated methods

545ff9d

sidharthachatterjee previously approved these changes May 19, 2020

View reviewed changes

sidharthachatterjee changed the title ~~feat(gatsby-link,gatsby): Add support for relative links~~ feat(gatsby): Add support for relative links May 19, 2020

Add docs for relative links

5bedec1

ascorbic dismissed sidharthachatterjee’s stale review via 5bedec1 May 20, 2020 15:24

ascorbic requested a review from a team as a code owner May 20, 2020 15:24

Typo

72df2ad

AishaBlake reviewed May 20, 2020

View reviewed changes

docs/docs/linking-between-pages.md Outdated Show resolved Hide resolved

Clarify

f38d4f5

ascorbic requested a review from AishaBlake June 1, 2020 14:12

Ekwuno approved these changes Jun 2, 2020

View reviewed changes

sidharthachatterjee self-requested a review June 2, 2020 11:48

sidharthachatterjee approved these changes Jun 2, 2020

View reviewed changes

ascorbic merged commit e2c6cf2 into master Jun 2, 2020

delete-merged-branch bot deleted the feat/relative-links branch June 2, 2020 11:49

herecydev mentioned this pull request Jun 9, 2020

LocalizedLink either adding two slashes or viewing link as the locale prefix herecydev/gatsby-plugin-internationalization#9

Closed

miedziak mentioned this pull request Jun 11, 2020

npm package update broke links in header bar build-trust/ockam-website#159

Closed

sidharthachatterjee mentioned this pull request Jun 16, 2020

Add support for relative links with @reach/router #6945

Closed

robinmetral reviewed Jun 20, 2020

View reviewed changes

LekoArts mentioned this pull request Jun 22, 2020

fix(using-i18n): Add slash to make absolute path, not relative #25180

Merged

LekoArts mentioned this pull request Jul 3, 2020

Internal Link Routing for Tags and Previous/Next Buttons Not Working Properly #25500

Closed

jestapinski mentioned this pull request Sep 15, 2020

DEVHUB-220: Relative Link Fix mongodb/devhub#508

Merged

boulaffasae mentioned this pull request Oct 8, 2020

Navbar links rise a problem jamstack-cms/jamstack-ecommerce#16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gatsby): Add support for relative links #24054

feat(gatsby): Add support for relative links #24054

ascorbic commented May 13, 2020

blainekasten May 13, 2020

ascorbic May 13, 2020

blainekasten left a comment

pvdz left a comment

pvdz May 13, 2020 •

edited

Loading

DSchau May 13, 2020

ascorbic May 13, 2020

DSchau May 13, 2020 •

edited

Loading

ascorbic May 13, 2020

pvdz May 13, 2020

pvdz commented May 13, 2020

pieh commented May 13, 2020

ascorbic commented May 15, 2020

sidharthachatterjee commented May 19, 2020

sidharthachatterjee commented May 19, 2020

sidharthachatterjee left a comment

wardpeet commented May 19, 2020 •

edited

Loading

sidharthachatterjee commented May 19, 2020

AishaBlake commented May 19, 2020 •

edited

Loading

Ekwuno left a comment

sidharthachatterjee left a comment

robinmetral Jun 20, 2020 •

edited

Loading

ascorbic Jun 20, 2020

robinmetral Jun 20, 2020

ascorbic Jun 20, 2020

fabianhaglund commented Jul 9, 2020

	: [__PATH_PREFIX__].concat([path.replace(/^\//, ``)]).join(`/`)
	: `${__PATH_PREFIX__}/${path.startsWith('/')?path.slice(1):path`

	if (process.env.NODE_ENV !== `production` && !isLocalLink(to)) {
	console.warn(
	`External link ${to} was detected in a Link component. Use the Link component only for internal links. See: https://gatsby.dev/internal-links`
	)
	}

feat(gatsby): Add support for relative links #24054

feat(gatsby): Add support for relative links #24054

Conversation

ascorbic commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blainekasten left a comment

Choose a reason for hiding this comment

pvdz left a comment

Choose a reason for hiding this comment

pvdz May 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DSchau May 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdz commented May 13, 2020

pieh commented May 13, 2020

ascorbic commented May 15, 2020

sidharthachatterjee commented May 19, 2020

sidharthachatterjee commented May 19, 2020

sidharthachatterjee left a comment

Choose a reason for hiding this comment

wardpeet commented May 19, 2020 • edited Loading

sidharthachatterjee commented May 19, 2020

AishaBlake commented May 19, 2020 • edited Loading

Ekwuno left a comment

Choose a reason for hiding this comment

sidharthachatterjee left a comment

Choose a reason for hiding this comment

robinmetral Jun 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianhaglund commented Jul 9, 2020

pvdz May 13, 2020 •

edited

Loading

DSchau May 13, 2020 •

edited

Loading

wardpeet commented May 19, 2020 •

edited

Loading

AishaBlake commented May 19, 2020 •

edited

Loading

robinmetral Jun 20, 2020 •

edited

Loading